e2e/e2etests/test_migrate_erc20_custody_funds.go (2)

38-39: Enhance error message clarity in event validation.

The error message could be more descriptive to help with debugging failures.

Consider this improvement:

-require.True(r, ok, "no EventERC20CustodyFundsMigration in %s", res.TxHash)
+require.True(r, ok, "expected EventERC20CustodyFundsMigration event not found in transaction %s", res.TxHash)

---

Line range hint 53-54: Consider refactoring the artificial state modification.

The comment indicates that the ERC20 custody address is being modified to prevent accounting check failures. This might mask potential issues and makes the test less reliable.

Consider:

1. Adding validation before the modification to ensure the migration was successful
2. Creating a separate test case for accounting validation
3. Documenting why this modification is necessary in more detail

e2e/e2etests/test_pause_erc20_custody.go (2)

35-38: LGTM! Consider enhancing the error message.

The event handling implementation is type-safe and clear. The error message could be more descriptive to aid debugging.

-require.True(r, ok, "no EventERC20CustodyPausing in %s", res.TxHash)
+require.True(r, ok, "expected EventERC20CustodyPausing event not found in transaction %s", res.TxHash)

---

40-47: LGTM! Consider enhancing error handling.

The CCTX handling logic is sound, with appropriate error checking and logging.

-require.NoError(r, err)
+require.NoError(r, err, "failed to fetch CCTX with index %s", event.CctxIndex)

e2e/e2etests/test_whitelist_erc20.go (1)

Line range hint 13-142: Consider breaking down the test into smaller, focused sub-tests.

The test function handles multiple concerns (deployment, whitelisting, verification, deposits, withdrawals). Consider refactoring into smaller sub-tests using t.Run() for better maintainability and clearer failure isolation.

Example structure:

func TestWhitelistERC20(r *runner.E2ERunner, _ []string) {
    // Common setup
    erc20Addr, cleanup := setupERC20(r)
    defer cleanup()

    t.Run("Whitelist", func(t *testing.T) {
        // Whitelisting specific tests
    })

    t.Run("Deposit", func(t *testing.T) {
        // Deposit specific tests
    })

    t.Run("Withdraw", func(t *testing.T) {
        // Withdraw specific tests
    })
}

This structure would:

1. Improve test failure isolation
2. Make the test flow easier to understand
3. Allow for parallel test execution if needed
4. Facilitate adding new test cases

e2e/runner/v2_migration.go (2)

152-154: LGTM! Consider enhancing the error message.

The transition to strictly typed events improves type safety and refactoring confidence. The error message could be more descriptive to aid debugging.

-    require.True(r, ok, "no EventERC20CustodyFundsMigration in %s", res.TxHash)
+    require.True(r, ok, "failed to find EventERC20CustodyFundsMigration during ERC20 custody pause operation (tx: %s)", res.TxHash)

---

191-193: LGTM! Consider extracting the event handling logic.

The event handling logic is duplicated from the previous segment. Consider extracting this into a helper method to maintain DRY principles and ensure consistent error handling.

func (r *E2ERunner) extractMigrationEventCCTXIndex(res *sdk.TxResponse, operation string) string {
    migrationEvent, ok := txserver.EventOfType[*crosschaintypes.EventERC20CustodyFundsMigration](res.Events)
    require.True(r, ok, "failed to find EventERC20CustodyFundsMigration during %s operation (tx: %s)", operation, res.TxHash)
    return migrationEvent.CctxIndex
}

Usage:

-    migrationEvent, ok := txserver.EventOfType[*crosschaintypes.EventERC20CustodyFundsMigration](res.Events)
-    require.True(r, ok, "no EventERC20CustodyFundsMigration in %s", res.TxHash)
-    cctxIndex = migrationEvent.CctxIndex
+    cctxIndex = r.extractMigrationEventCCTXIndex(res, "ERC20 custody migration")

e2e/e2etests/test_migrate_chain_support.go (1)

170-173: LGTM! Clean transition to typed event handling.

The implementation correctly transitions from direct attribute fetching to using strictly typed events, which aligns with the PR objectives. The error handling is robust with a descriptive message that includes the transaction hash for debugging.

Consider enhancing the error message to be more specific about what went wrong:

-	require.True(r, ok, "no EventERC20Whitelist in %s", res.TxHash)
+	require.True(r, ok, "failed to extract EventERC20Whitelist from transaction %s", res.TxHash)

Configuration used: .coderabbit.yaml
Review profile: CHILL

-require.True(r, ok, "no EventERC20Whitelist in %s", res.TxHash)
+require.True(r, ok, "expected EventERC20Whitelist not found in transaction %s response", res.TxHash)

#!/bin/bash
# Description: Check for consistent usage of EventOfType and direct attribute fetching

# Test 1: Look for any remaining direct attribute fetching
rg "FetchAttributeFromTxResponse.*whitelist_cctx_index|zrc20_address"

# Test 2: Verify consistent EventERC20Whitelist usage
ast-grep --pattern 'EventOfType[*crosschaintypes.EventERC20Whitelist]'

#!/bin/bash
# Let's search for all EventOfType usage patterns to ensure consistent event handling
rg "EventOfType\[.*\]" -A 2

# Also check for any remaining direct attribute fetching specifically in test files
rg "FetchAttributeFromTxResponse" --type go --glob "*test*.go"

 erc20zrc20Addr := event.Zrc20Address
 whitelistCCTXIndex := event.WhitelistCctxIndex
+require.NotEmpty(r, erc20zrc20Addr, "zrc20 address should not be empty")
+require.Greater(r, whitelistCCTXIndex, uint64(0), "whitelist CCTX index should be positive")

e2e/e2etests/test_migrate_erc20_custody_funds.go (1)

Line range hint 53-54: Consider redesigning the final state validation.

The need to artificially set ERC20CustodyAddr to prevent accounting check failures suggests a potential design issue. This approach could mask real problems and makes the test less reliable.

Consider either:

1. Implementing proper state tracking that doesn't require artificial manipulation
2. Adding explicit validation of both old and new custody addresses' states
3. Refactoring the accounting check to handle custody address changes gracefully

e2e/e2etests/test_pause_erc20_custody.go (1)

35-38: LGTM! Consider enhancing the error message.

The event handling implementation is clean and type-safe, aligning well with the PR's objective of strict event typing.

Consider making the error message more descriptive:

-require.True(r, ok, "no EventERC20CustodyPausing in %s", res.TxHash)
+require.True(r, ok, "expected EventERC20CustodyPausing event not found in transaction %s", res.TxHash)

e2e/e2etests/test_whitelist_erc20.go (1)

46-49: LGTM! Consider enhancing the error message.

The transition to type-safe event extraction using EventOfType is a solid improvement. It provides better compile-time safety and makes the code more maintainable during refactors.

Consider making the error message more descriptive:

-require.True(r, ok, "no EventERC20Whitelist in %s", res.TxHash)
+require.True(r, ok, "expected EventERC20Whitelist event not found in transaction %s", res.TxHash)

e2e/e2etests/test_migrate_chain_support.go (2)

170-173: LGTM! Consider enhancing the error message.

The transition to strictly typed event handling using generics is a solid improvement. The code properly checks for event presence and includes error context.

Consider making the error message more descriptive:

-	require.True(r, ok, "no EventERC20Whitelist in %s", res.TxHash)
+	require.True(r, ok, "failed to extract EventERC20Whitelist from transaction %s", res.TxHash)

---

Line range hint 1-400: Consider architectural improvements to enhance test maintainability.

The test implementation could benefit from the following improvements:

1. Extract hardcoded values (e.g., EVM2RPCURL, chain IDs, amounts) into a configuration struct or constants file.
2. Break down the large test into smaller sub-tests using t.Run() for better isolation and readability.
3. Add detailed comments explaining the purpose and expected outcome of each test phase.

Would you like assistance in implementing these improvements?

cmd/zetae2e/local/local.go (2)

188-191: Enhance comment clarity for setup steps.

The implementation looks good, but the comment could be more descriptive about the emissions pool funding step.

Consider updating the comment to be more specific:

-	// run setup steps that do not require tss
+	// run pre-TSS setup steps (emissions pool funding)

---

Line range hint 76-76: Consider architectural improvements for better maintainability.

The implementation would benefit from the following improvements:

1. Address the TODO comment by splitting the test setup logic into separate files
2. Extract the emissions pool funding into a dedicated setup function
3. Consider using more idiomatic Go error handling patterns

Example refactor for the emissions pool setup:

// setupEmissionsPool handles the emissions pool funding setup
func (r *Runner) setupEmissionsPool(skipSetup bool) error {
    if skipSetup {
        return nil
    }
    return r.FundEmissionsPool()
}

This would make the main function more concise and easier to maintain:

-	// run pre-TSS setup steps (emissions pool funding)
-	if !skipSetup {
-		noError(deployerRunner.FundEmissionsPool())
-	}
+	noError(deployerRunner.setupEmissionsPool(skipSetup))

Also applies to: 188-191

e2e/txserver/zeta_tx_server.go (4)

Line range hint 196-218: Validate non-empty message slice in BroadcastTx.

The BroadcastTx function now accepts a variadic msgs ...sdktypes.Msg parameter. To prevent unintended behavior when no messages are provided (e.g., zero gas and fees), ensure that msgs is not empty before proceeding.

Apply this diff to add a validation check:

func (zts ZetaTxServer) BroadcastTx(account string, msgs ...sdktypes.Msg) (*sdktypes.TxResponse, error) {
+	if len(msgs) == 0 {
+		return nil, errors.New("no messages provided to BroadcastTx")
+	}
	// Retrieve account number and sequence
	...
}

---

421-483: Review dependency on external package github.com/samber/lo.

The use of lo.FilterMap enhances code readability but introduces an external dependency. Evaluate if this dependency aligns with project policies and whether standard library alternatives (e.g., traditional loops) could suffice to reduce dependency overhead.

If acceptable, proceed; otherwise, refactor using standard constructs:

-deployMsgsIface := lo.FilterMap(
-	deployMsgs,
-	func(msg *fungibletypes.MsgDeployFungibleCoinZRC20, _ int) (sdktypes.Msg, bool) {
-		if skipChain(msg.ForeignChainId) {
-			return nil, false
-		}
-		return msg, true
-	},
-)

+var deployMsgsIface []sdktypes.Msg
+for _, msg := range deployMsgs {
+	if !skipChain(msg.ForeignChainId) {
+		deployMsgsIface = append(deployMsgsIface, msg)
+	}
+}

---

664-677: Enhance error handling in EventsOfType.

Currently, parsing errors are silently ignored. Logging or aggregating these errors can aid in diagnosing issues with event parsing, especially when expected events are not found.

Consider collecting errors:

func EventsOfType[T proto.Message](events []abci.Event) ([]T, bool) {
	var filteredEvents []T
+	var parseErrors []error
	for _, ev := range events {
		pEvent, err := sdktypes.ParseTypedEvent(ev)
		if err != nil {
+			parseErrors = append(parseErrors, err)
			continue
		}
		if typedEvent, ok := pEvent.(T); ok {
			filteredEvents = append(filteredEvents, typedEvent)
		}
	}
+	if len(filteredEvents) == 0 && len(parseErrors) > 0 {
+		// Optionally log or handle parseErrors
+	}
	return filteredEvents, len(filteredEvents) > 0
}

---

679-691: Consistent use of singular and plural in function names.

The function EventOfType fetches a single event, while EventsOfType fetches multiple events. Ensure that this naming convention is consistently applied throughout the codebase for clarity.

Confirm that all usages align with this convention, and consider adding documentation comments to clarify the purpose of each function.

e2e/e2etests/test_migrate_erc20_custody_funds.go (1)

41-41: 🛠️ Refactor suggestion

Consider adding context timeout for CCTX query.

While the implementation is correct, adding a timeout would prevent potential test hangs during network issues.

-	cctxRes, err := r.CctxClient.Cctx(r.Ctx, &crosschaintypes.QueryGetCctxRequest{Index: event.CctxIndex})
+	ctx, cancel := context.WithTimeout(r.Ctx, 30*time.Second)
+	defer cancel()
+	cctxRes, err := r.CctxClient.Cctx(ctx, &crosschaintypes.QueryGetCctxRequest{Index: event.CctxIndex})

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	ctx, cancel := context.WithTimeout(r.Ctx, 30*time.Second)
	defer cancel()
	cctxRes, err := r.CctxClient.Cctx(ctx, &crosschaintypes.QueryGetCctxRequest{Index: event.CctxIndex})

e2e/e2etests/test_pause_erc20_custody.go (2)

65-68: 🛠️ Refactor suggestion

Consider extracting a helper function to reduce duplication.

The event handling logic is duplicated between pause and unpause operations. Consider extracting a helper function to improve maintainability.

func assertERC20CustodyPausingEvent(t *testing.T, events []abci.Event, expectedPauseStatus bool, txHash string) *crosschaintypes.EventERC20CustodyPausing {
    event, ok := txserver.EventOfType[*crosschaintypes.EventERC20CustodyPausing](events)
    require.True(t, ok, "expected EventERC20CustodyPausing event not found in transaction %s", txHash)
    require.Equal(t, expectedPauseStatus, event.Pause, "unexpected pause status")
    return event
}

Usage:

-event, ok = txserver.EventOfType[*crosschaintypes.EventERC20CustodyPausing](res.Events)
-require.True(r, ok, "no EventERC20CustodyPausing in %s", res.TxHash)
-require.False(r, event.Pause, "should be unpaused")
+event = assertERC20CustodyPausingEvent(r.T, res.Events, false, res.TxHash)

---

70-70: 🛠️ Refactor suggestion

Consider extending the helper function to include CCTX handling.

The CCTX handling logic is also duplicated. Consider extending the helper function to include CCTX operations.

func handleERC20CustodyPausingEvent(
    t *testing.T,
    r *runner.E2ERunner,
    events []abci.Event,
    expectedPauseStatus bool,
    txHash string,
    operation string,
) *crosschaintypes.CrossChainTx {
    event := assertERC20CustodyPausingEvent(t, events, expectedPauseStatus, txHash)
    
    cctxRes, err := r.CctxClient.Cctx(r.Ctx, &crosschaintypes.QueryGetCctxRequest{Index: event.CctxIndex})
    require.NoError(t, err)
    
    cctx := cctxRes.CrossChainTx
    r.Logger.CCTX(*cctx, operation)
    
    r.WaitForMinedCCTXFromIndex(event.CctxIndex)
    return cctx
}

Usage:

-cctxRes, err = r.CctxClient.Cctx(r.Ctx, &crosschaintypes.QueryGetCctxRequest{Index: event.CctxIndex})
-require.NoError(r, err)
-
-cctx = cctxRes.CrossChainTx
-r.Logger.CCTX(*cctx, "unpausing")
-
-r.WaitForMinedCCTXFromIndex(event.CctxIndex)
+cctx = handleERC20CustodyPausingEvent(r.T, r, res.Events, false, res.TxHash, "unpausing")

Also applies to: 77-77

e2e/runner/v2_migration.go (1)

191-193: 🛠️ Refactor suggestion

Consider extracting event handling into a helper function.

This event handling pattern is duplicated. Consider creating a helper function to reduce duplication and improve maintainability:

func (r *E2ERunner) getMigrationEventCctxIndex(res *sdk.TxResponse) string {
    migrationEvent, ok := txserver.EventOfType[*crosschaintypes.EventERC20CustodyFundsMigration](res.Events)
    require.True(r, ok, "no EventERC20CustodyFundsMigration in %s", res.TxHash)
    return migrationEvent.CctxIndex
}

Then use it in both locations:

cctxIndex := r.getMigrationEventCctxIndex(res)

e2e/txserver/zeta_tx_server.go (3)

560-573: ⚠️ Potential issue

Avoid using Must methods that may cause panics.

In InitializeLiquidityCaps, using MustGetAccountAddressFromName can lead to panics if the account name is not found. Replace it with GetAccountAddressFromName and handle the error to improve robustness.

Apply this diff to handle errors gracefully:

func (zts ZetaTxServer) InitializeLiquidityCaps(zrc20s ...string) error {
	liquidityCap := sdktypes.NewUint(1e18).MulUint64(1e12)

	msgs := make([]sdktypes.Msg, len(zrc20s))
+	accountAddr, err := zts.GetAccountAddressFromName(utils.OperationalPolicyName)
+	if err != nil {
+		return fmt.Errorf("failed to get account address: %w", err)
+	}
	for i, zrc20 := range zrc20s {
		msgs[i] = fungibletypes.NewMsgUpdateZRC20LiquidityCap(
-			zts.MustGetAccountAddressFromName(utils.OperationalPolicyName),
+			accountAddr,
			zrc20,
			liquidityCap,
		)
	}
	_, err := zts.BroadcastTx(utils.OperationalPolicyName, msgs...)
	return err
}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

// InitializeLiquidityCaps initializes the liquidity cap for the given coin with a large value
func (zts ZetaTxServer) InitializeLiquidityCaps(zrc20s ...string) error {
	liquidityCap := sdktypes.NewUint(1e18).MulUint64(1e12)

	msgs := make([]sdktypes.Msg, len(zrc20s))
	accountAddr, err := zts.GetAccountAddressFromName(utils.OperationalPolicyName)
	if err != nil {
		return fmt.Errorf("failed to get account address: %w", err)
	}
	for i, zrc20 := range zrc20s {
		msgs[i] = fungibletypes.NewMsgUpdateZRC20LiquidityCap(
			accountAddr,
			zrc20,
			liquidityCap,
		)
	}
	_, err = zts.BroadcastTx(utils.OperationalPolicyName, msgs...)
	return err

---

361-363: 🛠️ Refactor suggestion

Augment error message when event is not found.

In DeploySystemContracts, if the EventSystemContractsDeployed is not present, consider providing additional context in the error message to facilitate debugging.

Apply this diff to enhance the error message:

if !ok {
-	return SystemContractAddresses{}, fmt.Errorf("no EventSystemContractsDeployed in %s", res.TxHash)
+	return SystemContractAddresses{}, fmt.Errorf("EventSystemContractsDeployed not found in transaction events for TxHash: %s", res.TxHash)
}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

	deployedEvent, ok := EventOfType[*fungibletypes.EventSystemContractsDeployed](res.Events)
	if !ok {
		return SystemContractAddresses{}, fmt.Errorf("EventSystemContractsDeployed not found in transaction events for TxHash: %s", res.TxHash)

---

217-218: 🛠️ Refactor suggestion

Adjust gas and fee calculations for accuracy.

Multiplying gas and fees by the number of messages may not account for the individual gas requirements of each message, especially if they vary significantly. Consider calculating the total gas and fees based on the sum of estimated gas for each message to ensure accurate fee allocation.

Refactor the gas and fee calculation:

// Initialize totalGas and totalFees
+totalGas := uint64(0)
+totalFees := sdktypes.NewCoins()

for _, msg := range msgs {
+	// Estimate gas for each message
+	gas, err := zts.txFactory.WithGas(0).Simulate(txBuilder.SetMsgs(msg))
+	if err != nil {
+		return nil, err
+	}
+	totalGas += gas
+}

// Set total gas and fees
-txBuilder.SetGasLimit(zts.txFactory.Gas() * uint64(len(msgs)))
+txBuilder.SetGasLimit(totalGas)
-txBuilder.SetFeeAmount(zts.txFactory.Fees().MulInt(sdktypes.NewInt(int64(len(msgs)))))
+txBuilder.SetFeeAmount(sdktypes.NewCoins(sdktypes.NewCoin("azeta", sdktypes.NewInt(int64(totalGas)*gasPrice))))

Ensure gasPrice is defined appropriately.

Committable suggestion skipped: line range outside the PR's diff.